Repository navigation
feat: Password support for /e/authorize endpoint - #1088
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
20839a3 to
7544ab9
Compare
9a54293 to
05cbe63
Compare
| when (e.error) { | ||
| is EmbeddedAuthError.InsufficientAuthorization -> { /* continuation → VerifyPassword */ } | ||
| EmbeddedAuthError.InvalidRequest -> { | ||
| /* the username was rejected (e.g. "Invalid username format.") — fix the input and identify again */ |
There was a problem hiding this comment.
After InvalidRequest the SDK clears the session, so calling identify again fails with NoActiveSession. Suggest: "fix the input and restart with authorize()", as in the InvalidRequest KDoc and authorize.md.
| return this | ||
| } | ||
|
|
||
| fun willReturnInvalidUsernameFormat(): EmbeddedAuthMockServer { |
There was a problem hiding this comment.
willReturnInvalidUsernameFormat() isn't used by any test. A test for identify(USERNAME) returning InvalidRequest (session cleared) would have caught the docs issue in password-authorize.md.
| } | ||
|
|
||
| @Test | ||
| public fun `verifyPassword await keeps the session for retry when the token exchange fails`(): Unit = runTest { |
There was a problem hiding this comment.
Question: this test keeps the session after a failed token exchange (the token response has no id_token) and retries verifyPassword with the same auth_session. That retry runs /e/authorize again to get a new code, so it only works if the server accepts the same auth_session after it has already issued a code. The mock server can't tell us that. Can we confirm it with the server team?
For a network error on the token call the code may still be unused, so keeping the session is right there. We'd like both SDKs to behave the same, so we can align Auth0.swift once this is confirmed.
There was a problem hiding this comment.
Good catch. Retrying with the same session is not correct .
| } | ||
|
|
||
| @Test | ||
| public fun `verifyPassword await should return credentials`(): Unit = runTest { |
There was a problem hiding this comment.
All the new verifyPassword tests use execute() or await(). Please add one test with start(callback) (success and failure), since that path goes through AdvancingRequest.start.
There was a problem hiding this comment.
Intentionally avoided. await internally calls start. We are just writing duplicate tests.
| /** The password is submitted as typed — never trimmed — and masked on screen. */ | ||
| @Composable | ||
| private fun VerifyPasswordCard(onVerify: (String) -> Unit) { | ||
| var password by rememberSaveable { mutableStateOf("") } |
There was a problem hiding this comment.
rememberSaveable writes the password to the saved-state Bundle. Please use remember for the password field; samples get copied into real apps.
| EMAIL, | ||
| /** A phone number. */ | ||
| PHONE, | ||
| /** A username. */ |
There was a problem hiding this comment.
The design doc says the username field can hold either a username or an email ({user_name} / {email}). This KDoc just says "A username." Could we say that an email is also valid, and mention it in password-authorize.md?
There was a problem hiding this comment.
There is already an email. The intention was password can be used with both email and username. will update the design doc to clear the confusion. But this is correct
| onSubmit = { viewModel.identifyUsername(it) }, | ||
| ) | ||
|
|
||
| NextAction.VerifyPassword -> VerifyPasswordCard( |
There was a problem hiding this comment.
The sample never reads reason, so a wrong password just shows the password card again with no message. authorize.md shows how to surface INVALID_IDENTIFIER_OR_PASSWORD, so the sample should do the same.
Changes
This PR adds the support for ROPG in the
/e/authorizeendpoint. On successful verification theauth0-serverreturns an authorization code which is exchanged for a valid set of Credentials.Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors